It looks like this can be defined this as:

<VisuallyHidden as="label" htmlFor={ `block-editor-inserter__search-${ instanceId }` }>
    { __( 'Search for a block' ) }
</VisuallyHidden>

which avoids the extra div around the label. I saw a few other places in the PR where that would be beneficial.

I noticed the VisuallyHidden docs don't mention this, could be a good follow up PR to update those docs 😄

To avoid wrapping the label in an unnecessary <div>, we should instead replace the label with <VisuallyHidden as="label" htmlFor={...} />

Is extra <div> a bad thing though? I'd say a clear distinction between <VisuallyHidden> and a <label> is more readable.

We might have to work on the docs, but it should be clear enough that <VisuallyHidden> will render as whatever element it's passed with the as prop 😅
As a general principle though, adding extra markup for code readability is not advisable because it has a direct impact on performance for the end user. It's best to keep markup to a minimum and use spacing and (non-rendering) comments where needed for clarification.

We need <VisuallyHidden as="legend"> here. A <legend> must be the first direct descendant of a <fieldset>, so we can't have any intermediate wrappers.

This doesn't feel right; if we need these styles here we should be applying them with whatever classname we're currently using to hide stuff from vision. Or we should be using <VisuallyHidden> if possible. Probably something to tackle in a follow-up PR though 😅. I don't think ResponsiveBlockControl is being used anywhere atm.

Unrelated to this changeset, but these snapshots are not very readable. I wonder why all the spacing has been removed 🤔

This presents a problem, because <figcaption> has to be a direct descendant of <figure>. I suspect the best way forward here will be to just pass the components-visually-hidden class to RichText directly. We have to support direct use of the class anyway because of the php files 🤷‍♀

It's a bit mad, but this should work:

<VisuallyHidden as={ RichText } tagName="figcaption" // other RichText props ... />

It might be cleanest to extract the logic into a separate component so that the props can be spread onto either RichText or VisuallyHidden.

But looking at the VisuallyHidden implementation, it doesn't look like it accepts additional class names. I may knock together a quick PR to fix that.

PR to better handle class names in VisuallyHidden - #20649

Yes, let's fix it :)

In general, you should be able to pass anything you want as as and all props should be passed down.

@talldan I think that makes sense! I added a helper component inline since it's pretty specific to this file, and I prepared a more general helper in #20928

This change and the one above will break form semantics; best leave things as they were.

While <div> and <span> have no semantic meaning, they do have user agent styling differences, so best not change this.

Does the user agent styling matters if <VisuallyHidden> is always visually hidden?

In practice it won't make a difference because the styles for hiding position the element absolutely, so it's removed from the flow, and it doesn't matter if its display type is block or inline.
But there's no good reason for making this change either, so best keep the changeset to what is strictly necessary 🙂

Same as above.

There might be some issues using component class name in this way, since a component's class name isn't meant to be used outside of the component.

This html is also going to be output to a WordPress site's frontend, so would the components styles be loaded? Not sure, I'd need to test it. 😄

It might be preferable for it to continue using the screen-reader-text global style or for the block to have its own visually hidden style.

It may also be an idea to split some of the changes that need more discussion into a separate PR so that they don't hold up the other changes.

It might be preferable for it to continue using the screen-reader-text global style or for the block to have its own visually hidden style.

Yes, correct. PHP part should be left as is.

I think #20928 would be a better solution here

Keep in mind that you still need to apply a different class name when it's visually hidden so there is going to be some extra logic involved.

Wouldn't the className be passed in anyway as part of richTextProps, or am I missing something?

I wanted to avoid passing className to <VisuallyHidden />, but when I looked into it more .block-gallery-caption shouldn't interfere with any styles we have there so I'll simplify this.

If we're keeping the original classname as per this thead then this needn't change either.

@tellthemachines, can you explain why we need still to render RichText when it's hidden visually? I see you introduced this behavior in #17101. In practice, it will never get focused because as soon as you move the focus to the Gallery block, then this condition will evaluate to false because isSelected will become true and the placeholder will show up when the caption is empty.

Yup, I did that so it'll show up on the list of form controls for screen reader users. So if you're using a screen reader, you can navigate straight to the caption element through the forms list, even if the gallery block isn't focused. Of course, as soon as you navigate to the element it becomes focused and visible.

I notice that this is inconsistent with how we're treating the citation in the Quote block (which disappears from the DOM when empty not focused) and how we treat empty Paragraph blocks (which have visible placeholder text at all times). I'm not 100% sure which of these solutions is best accessibility and usability-wise. Perhaps this could be discussed further in the weekly accessibility chat? It would be best to have a consistent behaviour for all these cases.

It's definitely something worth discussing with the bigger group 👍

It would be nice to be more consistent if other blocks always show placeholders then maybe we should follow that as well. However, I'm sure it's something that always has trade-offs. In the case of Paragraph without the placeholder, you could miss the fact that the block is even there when using mouse navigation :) On the other hand, if you would display caption placeholders for all images in the Gallery block then it would be very loaded. I think it also translates to the screen read case, too many controls exposed might make it harder to navigate to the currently selected block.

Update: a similar issue is being discussed in #15308 and seems to be going in the direction of making placeholders visible by default.